Name the debug contracts after the roles they fill - #356
Open
danielpourbakhsh wants to merge 1 commit into
Open
Conversation
The contracts went out in 1.0.28 with names that do not say what their implementers do, and nothing in this repository implements them yet, so the cost of correcting that is at its lowest right now. IDebugLaunchProvider becomes IDebugTargetPreparer and IDebugAdapter becomes IDebugSessionLauncher. Together they are one chain: the preparer brings a target up and produces a DebugLaunchRequest, the launcher decides whether it can serve that request and builds the session. "Launch" already ran through the request and through CanLaunch; the type in between was the only part of that family named after something else. "Adapter" was borrowed from DAP, where it means a process speaking a wire protocol - this one is in-process and speaks none, which the old summary admitted in its first line. The id the two agree on becomes BackendId, which is what it identifies and what its own documentation already called it. Take the breakpoint members off OneWare.Essentials.EditorExtensions.BreakPoint. While a published contract named that class, every change to the editor's margin model would have been a change to published API. Rather than add a second type, DebugStackFrame becomes DebugBreakPointFrame and serves both directions: it already answered the question a breakpoint asks - which place in the target - only reading instead of writing. Setting a breakpoint where the target is halted now needs no conversion, and the contracts folder has no reference pointing out of it. DebugLaunchRequest gains InitCommands so that whoever brings a target up can state what the backend cannot learn from the executable alone. The alternative in practice is a file next to the executable whose name both sides agree on without either contract saying so. RegisterValue becomes DebugRegisterValue, the last entity without the prefix. The namespace does not disambiguate for a caller who has imported it. This is a breaking change for anyone compiled against 1.0.28. Nothing in this repository implements these contracts, so the build here is unaffected, but PluginCompatibilityChecker compares only major, minor and build - a plugin built against a patch-level predecessor still loads and then fails at runtime. Shipping this under a minor bump would turn that into a clean refusal.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The contracts shipped in 1.0.28 with names that do not describe what their implementers actually do, and nothing in this repository implements them yet. This is therefore the lowest-cost point at which to correct the API.
IDebugLaunchProviderbecomesIDebugTargetPreparer, andIDebugAdapterbecomesIDebugSessionLauncher. Together, they form one chain: the preparer brings a target up and produces aDebugLaunchRequest; the launcher decides whether it can serve that request and builds the session. "Launch" already describes the request and theCanLaunchmethod, so the type in the middle was the only part of that family named after something else.The identifier the two contracts agree on becomes
BackendId, which is what it identifies and what its own documentation already calls it.The breakpoint members are removed from
OneWare.Essentials.EditorExtensions.BreakPoint.IDebugSessionis implemented by a backend, yet implementing it previously required referencing the editor's own breakpoint model. UI-BreakPointcannot express everything a session process nor a backend can accept: it represents a file and line, leaving no way to describe a breakpoint on an address or function.Rather than introduce a second type,
_DebugStackFrame_becomes_DebugBreakPointFrame_and serves both directions. It already answers the question a breakpoint asks — which location in the target — only for reading rather than writing. Setting a breakpoint at the location where the target is halted therefore requires no conversion, and the contracts folder no longer contains a reference pointing back to the editor.DebugLaunchRequestgainsInitCommands, allowing whoever brings the target up to specify information the (gdb)-backend cannot infer from the executable alone. The practical alternative is a file next to the executable —.gdbinit, in my case — whose name and location both sides have to agree on without either contract expressing that dependency. I would rather make that requirement explicit in the contract.RegisterValuebecomesDebugRegisterValueto match the naming of the other entities.This breaks any plugin compiled against the contracts as published. The only
one I am aware of is my own, and I will rebuild it against whatever version
this goes out in — so the practical impact is likely nil.